Painless: Move More Logic to PainlessLookup #32689
Conversation
jdconrad
added
:Core/Infra/Scripting
|
Pinging @elastic/es-core-infra |
| return handle; | ||
| try { | ||
| return painlessLookup.lookupRuntimeSetterMethodHandle(receiverClass, name); | ||
| } catch (IllegalArgumentException iae) { |
There was a problem hiding this comment.
I don't think we should use IAE as control flow. Can we return null if the the class is not found and base these special cases on that?
| @@ -174,4 +177,76 @@ public PainlessMethod lookupFunctionalInterfacePainlessMethod(Class<?> targetCla | |||
|
|
|||
| return functionalInterfacePainlessMethod; | |||
| } | |||
|
|
|||
| public PainlessMethod lookupRuntimePainlessMethod(Class<?> originalTargetClass, String methodName, int methodArity) { | |||
There was a problem hiding this comment.
Can you add at least a single line javadoc to these methods (and preferably the other ones as well here, although that could be in a followup)? It is getting difficult to understand what the differences in expected behavior are, eg based on a single word difference here of adding "runtime" to the method name, but taking the same args as lookupPainlessMethod above.
There was a problem hiding this comment.
I'd like to add documentation for all of these in a follow up PR as there are a few more changes lined up and I would prefer not to write it twice over the next couple weeks if that's okay.
|
@rjernst Thanks for the review. I changed all the methods in PainlessLookup to return null and react at the site of the calls instead of using exception handling as control flow. Ready for another round. |
|
@rjernst Thanks for the reviews! |
This moves some run-time lookups for methods and fields to the PainlessLookup.
This moves some run-time lookups for methods and fields to the PainlessLookup.